Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x-pack/filebeat/input/httpjson: Fix parseDate location offset #37738

Merged

Conversation

chemamartinez
Copy link
Contributor

@chemamartinez chemamartinez commented Jan 25, 2024

Proposed commit message

We encountered an issue from an SDH due to how the time.Parse function in Go handles timezone abbreviations. It interprets them based on the server's local time zone, which can lead to incorrect conversions if the server's local time zone doesn't match the time zone abbreviation in the string.

Since this is a limitation of the time.Parse function, we decided to add a new helper parseDateInTZ where users can apply the proper timezone when parsing dates. It accepts numeric offsets and IANA time zone names.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@chemamartinez chemamartinez self-assigned this Jan 25, 2024
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 25, 2024
Copy link
Contributor

mergify bot commented Jan 25, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @chemamartinez? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

Copy link
Contributor

mergify bot commented Jan 25, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fix-httpjson-parseDate-timezone upstream/fix-httpjson-parseDate-timezone
git merge upstream/main
git push upstream fix-httpjson-parseDate-timezone

@chemamartinez chemamartinez force-pushed the fix-httpjson-parseDate-timezone branch from 2827495 to b740082 Compare January 25, 2024 09:31
@chemamartinez chemamartinez added the Team:Service-Integrations Label for the Service Integrations team label Jan 25, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 25, 2024
@chemamartinez chemamartinez added Filebeat Filebeat x-pack Issues and pull requests for X-Pack features. bugfix backport-v8.12.0 Automated backport with mergify labels Jan 25, 2024
@chemamartinez chemamartinez force-pushed the fix-httpjson-parseDate-timezone branch from b740082 to 5788345 Compare January 25, 2024 09:46
@chemamartinez chemamartinez marked this pull request as ready for review January 25, 2024 09:46
@chemamartinez chemamartinez requested a review from a team as a code owner January 25, 2024 09:46
@elasticmachine
Copy link
Collaborator

❕ Build Aborted

There is a new build on-going so the previous on-going builds have been aborted.

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Start Time: 2024-01-25T08:52:18.873+0000

  • Duration: 60 min 13 sec

Test stats 🧪

Test Results
Failed 0
Passed 2819
Skipped 176
Total 2995

Steps errors 1

Expand to view the steps failures

Error signal
  • Took 0 min 0 sec . View more details here
  • Description: Error 'org.jenkinsci.plugins.workflow.steps.FlowInterruptedException'

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 25, 2024

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2024-02-20T18:23:51.082+0000

  • Duration: 136 min 3 sec

Test stats 🧪

Test Results
Failed 0
Passed 3320
Skipped 180
Total 3500

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced that this is the right solution. If we cannot parse the timezone because it is incorrectly using a non-IANA tzdb label or absolute fixed timezone, it should be set in the UI config.

@chemamartinez
Copy link
Contributor Author

I'm not convinced that this is the right solution. If we cannot parse the timezone because it is incorrectly using a non-IANA tzdb label or absolute fixed timezone, it should be set in the UI config.

@efd6 thanks for taking a look!

Since this is fixing a value template, that parses dates that can come from anywhere at any timezone, how would we set the timezone in the UI config? Do you mean forcing users that use this value template to define what's the timezone of the incoming dates?

On the other hand, do you see any particular issue that makes this change undesirable? I agree this may not be the optimal solution, even the Go developers seem to have been debating the issue for quite some time:
golang/go#56528
golang/go#63345
For now, if no native solution is provided, the time.Parse docs suggests using ParseInLocation when facing this kind of issues, and it works at least for the most common cases.

Please let me know your thoughts about this. Thanks!

@efd6
Copy link
Contributor

efd6 commented Jan 28, 2024

The commentary in go.dev/issue/63345 goes over the issues with this very nicely and explains exactly why this is undesirable. The caveat in the final paragraph of time.Parse is also informative about why this is a dangerous action to take (briefly, if the agent is running in a location that does not know the abbreviation, the tzoffset will be zeroed, or worse if it is in another location where the abbreviation means something else it will be definitively incorrect).

In practice any particular instance of HTTPJSON would likely only be collecting data from a single tz, so if this is not parseable, a single configuration location would be enough to specify that time be handled in that singular location. Maybe it would be helpful to have a parseTimeInLocation template helper.

If this change here is added, it needs documentation making the sharp edges clear and suggesting that users configure time-offset timezone specs if possible.

Copy link
Contributor

mergify bot commented Jan 31, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fix-httpjson-parseDate-timezone upstream/fix-httpjson-parseDate-timezone
git merge upstream/main
git push upstream fix-httpjson-parseDate-timezone

1 similar comment
Copy link
Contributor

mergify bot commented Feb 5, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fix-httpjson-parseDate-timezone upstream/fix-httpjson-parseDate-timezone
git merge upstream/main
git push upstream fix-httpjson-parseDate-timezone

@chemamartinez chemamartinez force-pushed the fix-httpjson-parseDate-timezone branch from 5788345 to b767c90 Compare February 20, 2024 18:23
@chemamartinez chemamartinez requested a review from a team as a code owner February 20, 2024 18:23
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

cc @chemamartinez

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

cc @chemamartinez

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

cc @chemamartinez

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

cc @chemamartinez

@chemamartinez
Copy link
Contributor Author

In practice any particular instance of HTTPJSON would likely only be collecting data from a single tz, so if this is not parseable, a single configuration location would be enough to specify that time be handled in that singular location. Maybe it would be helpful to have a parseTimeInLocation template helper.

If this change here is added, it needs documentation making the sharp edges clear and suggesting that users configure time-offset timezone specs if possible.

@efd6 I added a new value template parseDateInTZ and updated the docs intending to clarify edge cases and this timezone issue. Regarding the implementation of parseDateInTZ, I had two different approaches depending on what we would prefer:

  1. Using time.ParseInLocation gives preference to the implicit timezone in the provided date over the specified timezone.
  2. Using time.Parse after a manual assignment of the location. That way we prioritize the given one.

I chose the last option, let me know if you have second thoughts on this.

@chemamartinez chemamartinez requested a review from efd6 February 20, 2024 18:31
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

cc @chemamartinez

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

cc @chemamartinez

@efd6
Copy link
Contributor

efd6 commented Feb 20, 2024

I chose the last option

I agree.

@chemamartinez chemamartinez added backport-v8.13.0 Automated backport with mergify and removed backport-v8.12.0 Automated backport with mergify labels Feb 21, 2024
@chemamartinez chemamartinez merged commit 8214f9f into elastic:main Feb 21, 2024
38 of 39 checks passed
mergify bot pushed a commit that referenced this pull request Feb 21, 2024
Add a new value template helper `parseDateInTZ` where users can apply the proper timezone when parsing dates, avoiding the limitations of `parseDate` with timezone abbreviations. It accepts numeric offsets and IANA time zone names.

(cherry picked from commit 8214f9f)
chemamartinez added a commit that referenced this pull request Mar 6, 2024
… location offset (#38085)

* x-pack/filebeat/input/httpjson: Fix parseDate location offset (#37738)

Add a new value template helper `parseDateInTZ` where users can apply the proper timezone when parsing dates, avoiding the limitations of `parseDate` with timezone abbreviations. It accepts numeric offsets and IANA time zone names.

(cherry picked from commit 8214f9f)

* Fix changelog

---------

Co-authored-by: Chema Martínez <chema.martinez@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.13.0 Automated backport with mergify bugfix Filebeat Filebeat Team:Service-Integrations Label for the Service Integrations team x-pack Issues and pull requests for X-Pack features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants